feat(auth): Introduce Regional Access Boundaries for GoogleCredentials#12787
feat(auth): Introduce Regional Access Boundaries for GoogleCredentials#12787vverman wants to merge 5 commits intogoogleapis:regional-access-boundariesfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Regional Access Boundaries (RAB) across various credential types, including Compute Engine, Service Account, and External Account credentials. It adds a RegionalAccessBoundaryManager to handle the lifecycle, caching, and asynchronous refreshing of RAB information, which is then included in outgoing API requests via the x-allowed-locations header. Feedback includes a recommendation to use try-with-resources for HttpResponse to prevent connection leaks, a suggestion to maintain map immutability in getAdditionalHeaders to avoid regressions, and a request to use a shared, bounded ExecutorService instead of creating new threads for background refreshes.
| RegionalAccessBoundaryResponse json; | ||
| try { | ||
| HttpResponse response = request.execute(); | ||
| String responseString = response.parseAsString(); | ||
| JsonParser parser = OAuth2Utils.JSON_FACTORY.createJsonParser(responseString); | ||
| json = parser.parseAndClose(RegionalAccessBoundaryResponse.class); | ||
| } catch (IOException e) { | ||
| throw new IOException( | ||
| "RegionalAccessBoundary: Failure while getting regional access boundaries:", e); | ||
| } |
There was a problem hiding this comment.
The HttpResponse object is not closed, which can lead to a connection leak. It is recommended to use a try-with-resources block to ensure the response is closed properly. Additionally, the parsing logic can be simplified by using response.parseAs(RegionalAccessBoundaryResponse.class).
RegionalAccessBoundaryResponse json;
try (HttpResponse response = request.execute()) {
json = response.parseAs(RegionalAccessBoundaryResponse.class);
} catch (IOException e) {
throw new IOException(
"RegionalAccessBoundary: Failure while getting regional access boundaries:", e);
}References
- When managing a collection of closeable resources (e.g., scopes), ensure they are closed in the reverse order of their creation (LIFO). The implementation must be exception-safe to prevent resource leaks, meaning all opened resources should be closed even if exceptions occur during their creation or closing.
There was a problem hiding this comment.
Will do a quick follow up PR which addresses this and other points if that's okay with you?
| @Override | ||
| protected Map<String, List<String>> getAdditionalHeaders() { | ||
| Map<String, List<String>> headers = super.getAdditionalHeaders(); | ||
| Map<String, List<String>> headers = new HashMap<>(super.getAdditionalHeaders()); | ||
|
|
||
| String quotaProjectId = this.getQuotaProjectId(); | ||
| if (quotaProjectId != null) { | ||
| return addQuotaProjectIdToRequestMetadata(quotaProjectId, headers); | ||
| } | ||
| return headers; | ||
| return addQuotaProjectIdToRequestMetadata(quotaProjectId, headers); | ||
| } |
There was a problem hiding this comment.
The current implementation of getAdditionalHeaders creates a mutable HashMap and returns it if quotaProjectId is null, which is a regression in safety from the previous implementation that always returned an unmodifiable map. This can be simplified by passing the result of super.getAdditionalHeaders() directly to addQuotaProjectIdToRequestMetadata, which already handles null project IDs correctly.
@Override
protected Map<String, List<String>> getAdditionalHeaders() {
return addQuotaProjectIdToRequestMetadata(getQuotaProjectId(), super.getAdditionalHeaders());
}| Thread refreshThread = new Thread(refreshTask, "RAB-refresh-thread"); | ||
| refreshThread.setDaemon(true); | ||
| refreshThread.start(); |
There was a problem hiding this comment.
Creating a new Thread for every background refresh is generally discouraged in library code as it can lead to excessive thread creation if many credential instances are used. It is better to use a shared, bounded ExecutorService to manage background tasks more efficiently.
References
- For safety, use a bounded thread pool (e.g., ThreadPoolExecutor with a defined maximum size) instead of an unbounded one (e.g., Executors.newCachedThreadPool()), even if the current logic seems to limit concurrent tasks.
# Conflicts: # google-auth-library-java/oauth2_http/javatests/com/google/auth/oauth2/ExternalAccountCredentialsTest.java # google-auth-library-java/oauth2_http/javatests/com/google/auth/oauth2/GoogleCredentialsTest.java
lqiu96
changed the title
| */ | ||
| package com.google.cloud.dataplex.v1; | ||
|
|
||
|
|
There was a problem hiding this comment.
I believe this is a linter change.
There was a problem hiding this comment.
This seems odd. Do you have an error message that required this change? Or was it a command you ran that ended up making this change?
lqiu96
left a comment
lqiu96
left a comment
There was a problem hiding this comment.
PR was approved in the auth library: googleapis/google-auth-library-java#1880
2 participants
Migrates RAB changes from the older repo -> https://github.com/googleapis/google-auth-library-java/tree/feat-tb-sa